Implement 'Create Webdriver' keyword - #245

Having a way to configure the underlying Webdriver better is definitely a good idea. Few comments below:

1. What was the reason this cannot be done in the normal Open Browser? Couldn't it get configuration parameters the same way?

2. Failing for TypeError if invalid argument name is given is fine in my opinion. This can be tuned later if needed.

3. I don't think normalizing argument names is a good idea. Could get really messy if there are arguments with similar names. Notice that Robot doesn't normalize argument names when you use argument=value syntax either.

4. Instead _name_value_pairs, could this keyword could also take *_config? That's easier to implement and, at least in my opinion, using

Keyword    name=value    name2=value2

is cleaner and easier to understand than

Keyword    name    value    name2    value2

Using **kwargs has two drawbacks, though:

• Requires RF 2.8+.
• Because user keywords don't support kwargs yet, it's not possible to accept any kwargs to a user keyword and pass them forwards.

Both of these limitations can be mitigated by accepting *varargs instead of, or in addition to, **kwargs and splitting given arguments from = sign.

Just wanted to add that I have seen this and I am reviewing it to see what I might be able to add to the conversation.

UPDATE: Selenium team seems to be welcome to having a more "standard" way to create WebDrivers.

Thank you so much for you input Pekka. Good software rarely, if ever, comes without peer review. I am also glad you are taking an interest in this project as it is so important to the RF universe.

I was coding under that idea that forcing users to upgrade to 2.8 (to use **kwargs) was out of the question. Given your comments and the lack of serious issues found in 2.8, maybe that should be reconsidered. Ed, Jeremy, and Ryan would have to be on board with that.

The reasons for not adding the capability to pass arguments to the init's in Open Browser were:

1. There is an overlap of argument names between Open Browser and some init's.
   desired_capabilities: Open Browser, Chrome, Opera, PhantomJS
   capabilities: Firefox, Ie
   This of course, means implementation and documentation is easier.
2. Adding *varargs to Open Browser would preclude ever adding named arguments to it in the future (again, operating under the assumption that **kwargs was out of the question).
3. In another thread, Ed seemed to agree that it was a good idea to have a separate keyword.
4. A URL parameter is inappropriate for native app testing (this is easily resolvable by telling people to send ${EMPTY} or ${None} and maybe even defaulting to one of those).

I agree with what you said about normalizing argument names. I just did not want to appear lazy or unwilling to make the code friendlier.

I'm not sure how we could accept *varargs and splitting on = would work. If the user is trying to pass in an object, would the object not be converted to a string? If we tried to use BuiltIn.register_run_keyword and parse the arguments inside Selenium2Library, would that not work if Selenium2Library was imported using the WITH NAME syntax and thus had a different name that Selenium2Library would not know about?

In summary, I think there is a way to alter Open Browser to handle extra arguments if we can overcome the issue of the argument naming collision and everyone is OK with making 2.8 the minimum version in a upcoming version of Selenium2Library. If we do decide to implement this keyword, it could always be deprecated later thanks to that functionality in RF.

As an aside, I realized when writing this keyword that the API docs linked in the keyword documentation did not mention Opera at all. I raised an issue and they fixed it within one hour! That was by Luke, the same person who seems open to having a way to create a WebDriver where all options are in a desired capabilities argument.

Thanks again, Pekka!

Hey Guys,

@ombre42 thanks for picking this up. I know I said that I would work on it and I've kind of dropped the ball. Anyways as you alluded to in your earlier post the web driver team seems to be open to standardising web driver. I'm working on the patch for selenium now and will hopefully have it out in a few days.

The idea is this. You should be able to pass everything into the web driver init function through the desired_capabilities. So I'm creating a function decorator that will decorate all the __init__ functions and it will look for entries in the desired_capabilities dictionary that have the same name as one of the keyword arguments of the init function and then it will pull the value out of the desired_capabilities and create a keyword argument for it that get's passed to the init function.

If that's not too confusing, the code is maybe a bit more confusing :) Well hopefully not. Code is below. I need to clean it up a bit, address a few edge case and most importantly deal with selenium's crazy fun build system. Which is crazy, but certainly isn't fun. :)

The other advantage of using the decorator is:

• "backwards compatibility" will be maintained (even though I think we should just admit the interfaces are wrong, break compatibility and move on)
• If / when new options are added to the driver __init__ functions they will automatically be handled, so we should be able to rely on using desired capabilities for everything for now and into the future.

Disadvantage:

• It may not be obvious how to put something like ChromeOptions, into desired_capabilities, but remember that a python dictionary is basically the same thing as JSON, and so if you think about it like JSON the solution becomes readily apparent.

Anyways here is a look at the code. I'm thinking if it get accepted, we won't have to make any changes at all to S2L, the existing Open Browser keyword should work... I think.

class AllowDesiredCapabilitesOverrides(object):
    '''This is a decorator class intended to decorate the __init__ method for
    all webdrivers.  It allows the caller to override any argument in the init
    method with similarly named key in desired_capabilities.  This way
    desired_capabilities can be used as a standard way to completely setup 
    / configure a webdriver instance.
    '''

    def __call__(self,f):
        '''Stores the various argument names for the function we are
        decorating removing the self argument.
        Also stores the index of the desired_capabilities argument.
        '''
        from inspect import getargspec
        decorated_func_args = getargspec(f)[0][1:]
        caps_arg_index = decorated_func_args.index("desired_capabilities")

        def wrap(init_self,*args,**kwargs):
            #find desired_capabilities -- thi needs to be expanded as some web drivers use
            #different argument names for desired_capabilities
            if caps_arg_index < len(args):
                caps = args[caps_arg_index]
            else:
                caps = kwargs.get("desired_capabilities") 
            if caps:
                for count, val in enumerate(decorated_func_args):
                    if caps.has_key(val):
                        kwargs[val] = caps.pop(val)
                 f(init_self,*args,**kwargs)
        return wrap

Decorating the __init__ function looks like this:

@AllowDesiredCapabilitesOverrides()
def __init__(self, executable_path="chromedriver", port=0,
                 chrome_options=None, service_args=None,
                 desired_capabilities=None, service_log_path=None):

What do you guys think?

I think that is a rather elegant solution. I had wondered about how to accomplish what you asked for in issue 6026 while being backwards compatible.
I do see issues with it. Without using the decorator package, a decorator like this would remove the doc unless you preserve it (e.g. will Sphinx not see Chrome's init's docstring?). You also change signature to be (*args, **kwargs). Are you going to try to add the decorator package as a dependency of Selenium? Sorry if I am pointing out the gritty details you intentionally omitted.

One suggestion I have is to use some sort of a prefix for the keys in the DesiredCapabilities dict.
init.service_args instead of just service_args. Maybe even phantomjs.init.service_args.
For the latter I would definitely make the decorator take a prefix argument.

@AllowDesiredCapabilitesOverrides(prefix="chrome.init")
def __init__(self, executable_path="chromedriver", port=0,
                 chrome_options=None, service_args=None,
                 desired_capabilities=None, service_log_path=None):

Overall, it looks like a great solution to the backwards-compatibility issue.

Few fast comments below:

1. *_kwargs not being supported by RF 2.7 and older is a problem. Looking at the download counts, it seems that RF 2.8 is much more widely used than older versions, though. In this situation I would say using *_kwargs is fine with new functionality when old tests are not affected. I don't think Se2Lib should even require RF 2.8 or newer, the keywords using *_kwargs could just have a note in their doc that using *_kwargs requires RF 2.8+.

Notice that this is just my opinion. You know this library and its users much better than I do, and I respect your decision either way.

2. Great to hear Selenium team is interested in standardizing WebDriver creation. I agree with @j1z0 that the current interfaces are wrong and should be changed, but starting that fight is probably not worth the effort. Using the desiderd_capabilities hack ought to be good enough for our purposes.

3. I agree with @ombre42 that the decorator approach proposed by @j1z0 is elegant. Automatically handling arguments added in the future and having all this automagic in a single place is great.

As @ombre42 pointed out, decorators like this loose __doc__, __name__ etc. A lighter version of using the decorator module is using functools.wraps that's available in the standard library. Unfortunately it cannot handle signatures, but I'm not sure how important that is. Selenium team definitely needs to be consulted is either having a new dependency or losing signatures acceptable.

@ombre42, @pekkaklarck,

Thanks for the feedback. In terms of the doc, and name bit, I haven't tried yet, but I was think I could just reset those in the decorator it self. I don't think it would require the decorator module and I'm trying not to add new module dependencies cause I don't know how the selenium team will respond. I even used monkey patches instead of mock in my unit testing to avoid adding a framework. :)

Anyways @ombre42, I do like the idea of adding an init argument, as I was going to have to try 3 or 4 different argument names to 'find' desired_capabilites. With the init argument I could just create a simple dictionary and key it that way. Although I'm not sure if we also need that argument as a prefix in the desired_capabilities dictionary... is that just to make it clear from the user perspective?

With regards to the signature change.... I guess let's ask the Selenium team and see what they say.

Anyways I was hopping to get some verification from you guys that this solution will indeed work for our needs. Which I think I'm getting, so I'll go ahead and proceed. Unfortunately I had a bit of a HDD crash (I'm on a friends computer right now). So it may take a couple more days till I get this all wrapped up, but I'll get it out as soon as possible and post it to the selenium forum to get feedback from their side.

With any luck they will accept and maybe in a week or so we will have something to work with.

I would agree with @pekkaklarck on not forcing all users to upgrade to RF 2.8. My concern, and the reason for suggesting a second keyword, was to try not being too disruptive with everyone's current tests. I can see a future where one is depreciated and then removed. I'm not completely against change but looking at reducing disruption while improving our long term roadmap.

If I am not too late in asking this what is the issue, as suggested here, with using RemoteWebDriver as a single constructor? @j1z0 if I'm not mistaken you said it was the introduction of extra latency for which I would agree this is not desirable.

@emanlove -- Extra latency may be one thing yes, but I think the main reason with using RemoteWebDriver is it doesn't provide for the functionality we are trying to cater for. We are trying to provide a solution to the functionality not provided through desired_capabilities and thus not provided through RemoteWebDriver. So all those init parameters which are driver specific are not available through RemoteWebDriver because all you have in RemoteWebDriver is desired_capabilities.

For example if you wanted to load a Chrome extension for testing... can't do it through RemoteWebDriver. There are other examples, but honestly I'm drawing a blank right now. :)...

So the point of the desired_capabilites hack it in effect to allow us to treat everything with a standard interface, which would in effect be what using RemoteWebDriver is supposed to accomplish, but we could now do it using the native drivers.

What's odd though is jumping into the code it looks like, looking first at ChromeDriver, those oddball options get converted, using options.to_capabilities() [line 53] for Chrome, and then shoved into desired_capabilities. The others each do it differently (Firefox, IE, Opera, etc.).

I must admit I not sure what I am seeing and I am looking very narrow perspective so I trying to deepen my understanding here. @j1z0 your solution looks great. It just looks like a chicken and an egg problem (and I am a confused farmer :)

@j1z0 - Too bad about the HDD crash - I've been there before. There are several reasons for using a prefix, but here is one that makes it an absolute requirement - an existing conflict.
The Firefox WebDriver's init has a proxy argument that is assumed to be a webdriver.Proxy object.
'proxy' is also the key in DesiredCapabilities that is supposed to have a value of a JSON object describing a Proxy configuration. In other words, the Firefox WebDrvier would fail during initialization when it would try to call the add_to_capabilities() method on a dict that should have been left in the capabilities dict.

There are some changes we need to make to S2L. Currently the capabilities are passed down to _generic_make_browser and then ignored for local webdrivers.
While we are at it, we might as well fix #217 by modifying a copy of DesiredCapabilities.FIREFOX, etc. instead of modifying the original. I put in an issue today saying that I think it is bad to have an example with side effects like modifying webdriver.DesiredCapabilities.INTERNETEXPLORER.

@emanlove - your right about Chrome Options getting converted. As @ombre42 Firefox does something similar with the proxy object. But its not complete. For example you can't really specify a firefox_profile through desired capabilities, You can't even specify the timeout. In chrome you couldn't specify executable_path, service_args (which are flags to pass to chrome_driver) or log path. Really it's just Chrome, Firefox and PhantomJS that are deviant.

I agree it is a chicken and an egg thing, and again @ombre42 suggestion to put an init parameter does help clear the air. But now I'm actually starting to question some of my original assumptions. For example trying to pass in a complete JSON representation of a firefox_profile could prove to be a nightmare. Probably better to just pass in the constructor arguments. But then it all seems to start smelling like a giant hack. I don't know.. need to think more.. Brain hurts...

Ok thinking about this some more and just want to bounce around some idea. I looked through the issue list for S2L and here are the issues that are potentially related:
#215
#225
#214
#208
#204
#206
#139
#132
#100
#26
#18

Really they break down into four categories.

1. Specify firefox profiles.
2. Specify Chrome Options.
3. Specifying service args.
4. Proxies

There is also a request to implement Safari support which should be a no brainer now, that it's there in selenium.

Anyways if I try to think of the simplest solution that would work to solve those issues.

1. The ability to pass in a firefox profile is already there in selenium, we may need to fix a few issues on our side.
2. Could be solved by passing in desired_capabilities for local (not just remote) creation.
3. Service Args (could use the decorator above or we could simply add service args parameter to open browser.
4. Proxies same solution as for Chrome Options item 2 above.

The thing that is making me rethink my original solution is how do I know when somebody is passing in firefox profiles or service args? I would have to treat them as special cases, which is actually what they already are in that they are not part of desired_caps.

So maybe all we should do is try to standardise the naming for desired_capabilities in selenium and on our side add service_args to the open_browser keyword.

I wonder, and please correct me if I'm off base here, if we are trying to solve problems we don't actually have. Is this a case of YANGNI "Your Not Going to Need It".... I really don't know anymore. I'm running around in circle. I'm hopping you guys can set me straight. :)

I’d sure like the Safari support!

Thanks,
Martin

From: Jeremy Johnson [mailto:notifications@github.com]
Sent: Monday, October 28, 2013 5:05 PM
To: rtomac/robotframework-selenium2library
Subject: Re: [robotframework-selenium2library] Implement 'Create Webdriver' keyword (#245)

Ok thinking about this some more and just want to bounce around some idea. I looked through the issue list for S2L and here are the issues that are potentially related:
#215#215
#225#225
#214#214
#208#208
#204#204
#206#206
#139#139
#132#132
#100#100
#26#26
#18#18

Really they break down into four categories.

1. Specify firefox profiles.
2. Specify Chrome Options.
3. Specifying service args.
4. Proxies

There is also a request to implement Safari support which should be a no brainer now, that it's there in selenium.

Anyways if I try to think of the simplest solution that would work to solve those issues.

1. The ability to pass in a firefox profile is already there in selenium, we may need to fix a few issues on our side.
2. Could be solved by passing in desired_capabilities for local (not just remote) creation.
3. Service Args (could use the decorator above or we could simply add service args parameter to open browser.
4. Proxies same solution as for Chrome Options item 2 above.

The thing that is making me rethink my original solution is how do I know when somebody is passing in firefox profiles or service args? I would have to treat them as special cases, which is actually what they already are in that they are not part of desired_caps.

So maybe all we should do is try to standardise the naming for desired_capabilities in selenium and on our side add service_args to the open_browser keyword.

I wonder, and please correct me if I'm off base here, if we are trying to solve problems we don't actually have. Is this a case of YANGNI "Your Not Going to Need It".... I really don't know anymore. I'm running around in circle. I'm hopping you guys can set me straight. :)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/245#issuecomment-27261490.

or here is another idea. I could stick with the AllowDesiredCapabilitiesOverwrides decorator and have the decorator take an include / exclude parameter so firefox could say, exclude the firefox_profile and the proxy parameter from the overrides. That way if there are naming collisions or tricky to implement stuff we can just leave it out for the time being unless someone really needs it, then we can look at adding it later.

I think we should just meet at the pub and hash it out :)
If we are not going to implement a Create Webdriver keyword for advanced usage and we are going to block functionality like firefox_profile in Open Browser, then we should at least provide a documented, private method to add a driver to the cache that we condone the usage of for scenarios like using a Firefox profile with settings applied at runtime, for instance. Yes people can add a browser to the cache already, but it is done by calling an implementation specific method (register) on a private instance variable. That's my two cents.

I think we should just meet at the pub and hash it out :)

Hear! Hear!

If we are not going to implement a Create Webdriver keyword for
advanced usage and we are going to block functionality like
firefox_profile, then we should at least provide a documented, private
method to add a driver to the cache that we condone the usage of for
scenarios like using a Firefox profile with settings applied at
runtime, for instance. Yes people can add a browser to the cache
already, but it is done by calling an implementation specific method
(register) on a private instance variable. That's my two cents.

Let's work both fronts. On our side provide a Create Webdriver keyword
providing advance capabilities. This functionality in the future could
be merged or depreciated as needed. @ombre42: Can you rework your code
taking into consideration some of @pekkaklarck suggestions? We will let
@j1z0 work with the selenium team proposing a solution with all doing
code review of both tasks. And since I have just delegated away all the
work (minus code review) feel free all to reassign items back to me (and
I will cover the pub tab!)

Pekka did not respond to my comments. I can certainly add a **kwargs that
will work >=2.8, but I want to do what Pekka is suggesting and I do not see
how it is possible. Do you know how we could accept *varargs and split on =?

Here's my comments from issue:
I'm not sure how we could accept *varargs and splitting on = would work. If
the user is trying to pass in an object, would the object not be converted
to a string? If we tried to use BuiltIn.register_run_keyword and
parse *resolve *the arguments inside Selenium2Library, would
that not work if Selenium2Library was imported using the WITH NAME syntax
and thus had a different name that Selenium2Library would not know about?

Pub sounds great! Need to start planning conference trips or something similar to your direction. I hope we get this stuff sorted out before that, though. =)

I agree with @emanlove's two front approach. Good idea to have some solution now and continue working with a better solution.

Sorry @ombre42 for not replying to your comments. What I meant with splitting *varargs from '=' is best illustrated by an example:

def example(*varargs, **kwargs):
    if not all('=' in a for a in varargs):
        raise RuntimeError('Arguments must be given in `name=value` format')
    kwargs.update(a.split('=', 1) for a in varargs)
    print kwargs

If you use the above keyword like

Example    a=1    b=2    c=3

it will print out the same dict both with RF 2.7 and 2.8. Things get a bit more complicated if the keyword needs normal *varargs, but that's not too hard to handle either.

@ombre42: Rereading your comments I see what we are lacking in response to your question. You want to put in the arguments directly to the webdriver instantation but for Firefox, for example, the proxy argument is an object and not a string. Thus there is no way to pass a proxy object into the Create Webdriver keyword as you have outlined above.

I still looking at this but I am formulating that for the 'Create Webdriver' keyword will need to deal with string arguments and build up from there, possibly different for each browser/webdriver, each specific argument; assuming that is, as I am not there yet, we can do these for each argument. For example, for firefox, pass in the proxy argument something like

|Create Webdriver  | Firefox=firefox  | proxy=httpproxy=localhost:8888 |

or something similar. I am sorry for not providing a better example as I am fairly busy this week. But I did want to recognize the issue you've brought to light and we have yet to provide a satisfactory answer. I think in the end it is going to be a hack but with some improvements with selenium we should be able to provide a better solution.

I think I need to clarify what the purpose of Create Webdriver is. It is the keyword to use when Open Browser cannot work. It is the keyword you can use when Konquerer gets it's own webdriver and Selenium2Library has not had a chance to support it via Open Browser (just like Safari).
It does not try to be friendly - it does not check your work.
If you are specifying a port, you better use ${42} not 42 - arguments are passed on as is.
I think the documentation for Create Webdriver could be a little more clear on this.

@pekkaklarck I actually meant to send that comment to Ed, hoping I had missed something. Thank you for responding, and I am sorry I was not more clear.
Forgive me if I am being verbose here, but I seem to have a communication problem (again, easily resolvable over a pint).

This is an example from my pull - and please ignore that there are ways to send proxy information without using the proxy helper class-this is just an example of some object as an argument:

${proxy}=  Evaluate  sys.modules['selenium.webdriver'].Proxy()  sys, selenium.webdriver
${proxy.http_proxy}=  Set Variable  localhost:7777
Create Webdriver  Firefox  firefox  proxy  ${proxy}

If the caller used the following with named arguments instead, in RF < 2.8 (comes in as varargs not kwargs):
Create Webdriver Firefox firefox proxy=${proxy}

In the usual implementation, varargs[0] would be u'proxy=<selenium.webdriver.common.proxy.Proxy object at 0x014B5530>'

Pekka, I thought you were suggesting it was possible that we could make varargs in RF < 2.8 work like kwargs does in 2.8 with a little effort. But like I mentioned even if we told RF not to resolve variables before invoking Create Webdriver by using BuiltIn.register_run_keyword, wouldn't that fail to work if Selenium2Library was subclassed with a different name or imported with an alias since register_run_keyword uses the library name as its argument?

What I am saying is that it does not seem to be feasible to have kwargs-like support via varargs unless all values for arguments are literal strings or repr strings. For Create Webdriver, no processing of argments is going to occur because it is a generic gateway to the underlying Selenium code - not because I want keywords to be like this but because I see a real need for this.

I have not tried using register_run_keyword -- just going by the doc.

@emanlove - You seem to understand my problem, and thank you for acknowledging that.

@ombre42 Yes, variables containing non-strings work differently depending on do you use name=value with _varargs or *_kwargs. I was already adding a note about that to my previous comment, but the note got too verbose and I though it's not important in this case. I was wrong. And you are right this would be so much easier to communicate over a pint.

The question is, are there people who would be using non-string values that wouldn't be in RF 2.8 already or couldn't easily upgrade? Obviously the same can be asked about whole this new functionality. Perhaps it would be more straightforward to support only **kwargs. Support for RF < 2.8 can be added later if there is enough demand.

Just got another idea. Perhaps the keyword could work like:

def example(capabilities=None, **extra_capabilities):
    capabilities = capabilities or {}
    capabilities.update(extra_capabilities)
    # ....

Now advanced users could create a dictionary containing the capabilities and that would work regardless RF version. For most cases using **extra_capabilities would be more convenient, but obviously require 2.8.

@pekkaklarck I thought about having a dict argument instead of *name_value_pairs like you mentioned. For most use cases, the number of arg/value pairs will be one or two, so it seemed simpler to use variable arguments. The one downside to using the variable arguments in this keyword is it makes the optional argument alias become required. alias stays optional if you use keyword arguments.

@emanlove, @j1z0 let me know what you guys think.

@ombre42 Now that you added support for **init_kwargs, I'd say having another argument that can take args as a dict would work better than the current *name_value_pairs. Creating a dictionary is a bit extra work, but alias makes using *name_value_pairs a bit complicated too. Using the dict would probably be easier to document too. This isn't that big deal though, most users are likely to use **init_kwargs anyway.

I lean towards the idea of using a dictionary.

OK Pull request for Selenium is up. Here it is:

SeleniumHQ/selenium#134

Please up vote / commit / make sure it's ok and let's try to get it fixed in Selenium. Thanks guys.

Shall I merge @ombre42's changes in? It looks ready to go. I have been working on getting SauceLabs running but need some of these changes to fully utilize their test matrix.

I don't have any objections with the code but should the tests be all passing before it's merged?

Cheers,
Jeremy

On 14 Nov, 2013, at 23:20, Ed Manlove notifications@github.com wrote:

Shall I merge @ombre42's changes in? It looks ready to go. I have been working on getting SauceLabs running but need some of these changes to fully utilize their test matrix.

—
Reply to this email directly or view it on GitHub.

@j1z0 Thanks for pointing that out. The tests are still failing for browsers other than Firefox due to issues with the build environment.

I am not bothering to process ${DESIRED_CAPABILITIES}. Is that OK?

In Chrome, I cannot view the pages served by the test server other than the directory listing. This happens with Chrome outside of the tests (says the page is not responding and asks if I want to kill it). I'm using 31.0.1650.57. Is this a known issue?

hmm seems like my earlier message didn't get posted... weird... anyways basically I would suggest.

• Turn off IE / Safari in travis.yml cause they will never work on linux.
• Disable / set-to ignore the click_element_at_coordinates for firefox as this is a known issue with firefox.
• Those two will get the tests passing on travis anyways...
• Hope that someone can get the sauce labs stuff up and running through the travis build so we can turn on tests for all the other browsers.

As for the chrome issue you are having... don't know what to tell you, it works for me I got same Chrome version and I'm on OSX 10.9.